New "add-lines" configurator for simple file patching + importmap support #975

weaverryan · 2023-05-19T17:41:52Z

Hi!

Today, WebpackEncoreBundle's recipe contains the UX/Stimulus files. Soon, I will introduce a new StimulusBundle - https://github.com/weaverryan/stimulus-bundle - so that the WebpackEncoreBundle can be used without Stimulus and (more importantly) the stimulus_() functions can be used with AssetMapper (i.e. without Encore).

This makes the recipe setup more... interesting :). This PR adds 2 things:

importmap support in `JsonSynchronizer`

JsonSynchronizer now has 2 modes, based on the presence/absence of the importmap.php file. If that file is present, then:

A) A new symfony.importmap config is read from the bundle's package.json file and these are added to the importmap.php file by running the bin/console importmap:require command. The path: prefix is used to refer to a "local" file in the bundle. Sometimes the importmap entries will be different than what's needed for package.json, hence having both configs.
B) The controllers.json file is updated like normal

Also, a new symfony.needsPackageAsADependency config key was added specifically for StimulusBundle. If true, no file:/vendor/... package will be added to package.json when using Encore.

add-lines Configurator

The new add-lines configurator is able to add entire lines to the top, bottom of after_target of existing files (if they exist). Example usage:

"add-lines": [
    {
        "file": "webpack.config.js",
        "content": "\n    // enables the Symfony UX Stimulus bridge (used in assets/bootstrap.js)\n    .enableStimulusBridge('./assets/controllers.json')",
        "position": "after_target",
        "target": ".splitEntryChunks()"
    },
    {
        "file": "assets/app.js",
        "content": "import './bootstrap.js';",
        "position": "top",
        "warn_if_missing": true
    }
]

There is also a requires key to only run if another package is installed. This is needed because StimulusBundle will need a different assets/bootstrap.js based on if Encore vs AssetMapper is installed

Cheers!

weaverryan · 2023-05-22T00:44:07Z

This is ready for review 🖖

javiereguiluz

This looks OK to me. Thanks Ryan (and Kevin)!

nicolas-grekas · 2023-05-22T09:48:50Z

src/PackageJsonSynchronizer.php

+        if (file_exists($this->rootDir.'/importmap.php')) {
+            $this->synchronizeForAssetMapper($phpPackages);
+
+            return false;


should we configure also package.json in case both exist?

It's a good question - e.g. "if package.json exists, why not update it?". But one use-case will be that someone is using AssetMapper but has a small package.json file so that they can process Tailwind files or React files. In that case, we wouldn't want to update their package.json. So my thinking currently is: there's a boolean choice between Encore vs AssetMapper / importmap.php vs package.json.

nicolas-grekas · 2023-05-22T09:57:11Z

src/Configurator/AddLinesConfigurator.php

+                $fileContents = implode("\n", $lines);
+
+                if (!$targetFound) {
+                    // TODO warn?


display a message? Could not add lines after "%s" as no such string was found in "%s": $the-lines

Good catch! It looks like this now both here and if the file is missing (with wording differences, of course):

The user would only see this if warn_if_missing: true in manifest.json (or, of course, if they have verbosity high).

nicolas-grekas

LGTM. Do we need a rule to replace a line by another?

weaverryan · 2023-05-22T10:42:08Z

LGTM. Do we need a rule to replace a line by another?

Thank you! I didn't NEED the replace yet, so I skipped it - it could be a position: replace_target. I can add it now if you'd like. Otherwise, wait until we need it.

stof · 2023-05-22T11:47:54Z

src/Configurator/AddLinesConfigurator.php

+                throw new \InvalidArgumentException(sprintf('The "file" key is required for the "add-lines" configurator for recipe "%s".', $recipe->getName()));
+            }
+
+            if (isset($patch['requires']) && !$this->isPackageInstalled($patch['requires'])) {


should this be a string only or should we support a list of package names ?

My gut is: let's keep it simple as a string for now. We could expand it later to an array if needed - I can't think of a use-case yet.

But I CAN add it now if we want it - I've been trying to focus the scope.

turning it into an array later would require using a different key, to avoid making recipes incompatible with older versions of Flex that would break badly when receiving something else than a string. So if we don't support an array already, we should at least properly handled unsupported values.

Good point - I've added support for an array, as it turned out to be just as easy as checking for the type and throwing an exception.

…ryan) This PR was squashed before being merged into the 2.x branch. Discussion ---------- Adding Asset Mapper support + new StimulusBundle | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Tickets | None | License | MIT Adds: * A) Integration with new `symfony.importmap` key introduced to Flex in symfony/flex#975 * B) An "asset mapper" path in each bundle so that you can refer to the controller/files in that bundle. * C) Introduces new StimulusBundle (thanks to `@jmsche` for helping port from WebpackEncoreBundle). This includes the Twig `stimulus_*` functions + a revamped "helper" system for generating those attributes. * D) The new StimulusBundle also contains integration for AssetMapper to load custom controllers and `controllers.json`. Cheers! Commits ------- b8acf1a Adding Asset Mapper support + new StimulusBundle

stof · 2023-05-23T12:51:10Z

src/Configurator/AddLinesConfigurator.php

+            }
+
+            if (!isset($patch['content'])) {
+                throw new \InvalidArgumentException(sprintf('The "value" key is required for the "add-lines" configurator for recipe "%s".', $recipe->getName()));


should we throw or should we ignore the invalid recipe ? throwing would break the whole composer command.

All those validation rules should be implemented as part of the recipe validator for the CI of the recipes repo.

Good catch! I've made a PR to add the linting rules - symfony-tools/recipes-checker#14

I've also relaxed these to low-priority warnings (and skipping the rule). It will also help if we add more capabilities in the future to recipes, but they are perhaps on an older flex version (it won't work correctly, but no explosion).

…erryan) This PR was merged into the 2.x branch. Discussion ---------- Adding %PACKAGE% placeholder + fixing translations | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | Tickets | None | License | MIT Tracking with Flex updates at symfony/flex#975 Commits ------- 9d21d3a Adding %PACKAGE% placeholder + fixing translations

… (weaverryan) This PR was merged into the 6.3 branch. Discussion ---------- [AssetMapper] Add "=alias" syntax to importmap:require | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | yes-ish | Deprecations? | no | Tickets | None | License | MIT | Doc PR | Still TODO I'm doing final testing against all of the UX packages. There is one edge-case where we need to require a specific package path - `vue/dist/vue.esm-bundler.js` - but alias it (i.e. make its key) set to something else - `vue` - in `importmap.php`. We need the command to support this so that symfony/flex#975 can use it when installing UX packages. Hopefully the last thing I come across - I've done a LOT of testing at this point. Thanks! Commits ------- 27ecdb4 [AssetMapper] Adding an "alias" syntax to importmap:require

…ny/asset-mapper Co-authored-by: Ryan Weaver <ryan@symfonycasts.com>

fabpot · 2023-05-26T16:25:48Z

Thank you @weaverryan.

…ryan) This PR was squashed before being merged into the main branch. Discussion ---------- Adding linting for the new add-lines configurator Supports symfony/flex#975 Commits ------- 89b84e9 Adding linting for the new add-lines configurator

…ryan) This PR was squashed before being merged into the 2.x branch. Discussion ---------- Adding Asset Mapper support + new StimulusBundle | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Tickets | None | License | MIT Adds: * A) Integration with new `symfony.importmap` key introduced to Flex in symfony/flex#975 * B) An "asset mapper" path in each bundle so that you can refer to the controller/files in that bundle. * C) Introduces new StimulusBundle (thanks to `@jmsche` for helping port from WebpackEncoreBundle). This includes the Twig `stimulus_*` functions + a revamped "helper" system for generating those attributes. * D) The new StimulusBundle also contains integration for AssetMapper to load custom controllers and `controllers.json`. Cheers! Commits ------- b8acf1a2 Adding Asset Mapper support + new StimulusBundle

…erryan) This PR was merged into the 2.x branch. Discussion ---------- Adding %PACKAGE% placeholder + fixing translations | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | Tickets | None | License | MIT Tracking with Flex updates at symfony/flex#975 Commits ------- 9d21d3a3 Adding %PACKAGE% placeholder + fixing translations

kbond mentioned this pull request May 19, 2023

[WIP][POC] Add PatchConfigurator #926

Closed

4 tasks

This was referenced May 19, 2023

Adding new recipes for Encore, Stimulus & AssetMapper symfony/recipes#1202

Merged

Adding Asset Mapper support + new StimulusBundle symfony/ux#887

Merged

weaverryan force-pushed the patch-configurator branch from 528664b to 1d1e0f9 Compare May 22, 2023 00:40

javiereguiluz approved these changes May 22, 2023

View reviewed changes

nicolas-grekas reviewed May 22, 2023

View reviewed changes

stof reviewed May 22, 2023

View reviewed changes

stof approved these changes May 23, 2023

View reviewed changes

stof reviewed May 23, 2023

View reviewed changes

weaverryan mentioned this pull request May 23, 2023

Adding linting for the new add-lines configurator symfony-tools/recipes-checker#14

Merged

kbond approved these changes May 26, 2023

View reviewed changes

This was referenced May 26, 2023

[AssetMapper] Add "=alias" syntax to importmap:require symfony/symfony#50445

Merged

Adding %PACKAGE% placeholder + fixing translations symfony/ux#902

Merged

add AddLinesConfigurator + updating PackageJsonSynchronizer for symfo…

0b70eb3

…ny/asset-mapper Co-authored-by: Ryan Weaver <ryan@symfonycasts.com>

weaverryan force-pushed the patch-configurator branch from d37956c to 0b70eb3 Compare May 26, 2023 16:23

fabpot merged commit 49059a1 into symfony:1.x May 26, 2023

weaverryan deleted the patch-configurator branch May 26, 2023 16:26

alexander-schranz mentioned this pull request Jun 21, 2023

Add-lines patcher can not be converted from .yaml to .php #984

Open

mnocon mentioned this pull request Aug 10, 2023

Added Test Fixtures recipes ibexa/recipes-dev#85

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New "add-lines" configurator for simple file patching + importmap support #975

New "add-lines" configurator for simple file patching + importmap support #975

weaverryan commented May 19, 2023

weaverryan commented May 22, 2023

javiereguiluz left a comment

nicolas-grekas May 22, 2023

weaverryan May 22, 2023

nicolas-grekas May 22, 2023

weaverryan May 22, 2023

nicolas-grekas left a comment

weaverryan commented May 22, 2023

stof May 22, 2023

weaverryan May 22, 2023

stof May 23, 2023

weaverryan May 23, 2023

stof May 23, 2023

stof May 23, 2023

weaverryan May 23, 2023

fabpot commented May 26, 2023

New "add-lines" configurator for simple file patching + importmap support #975

New "add-lines" configurator for simple file patching + importmap support #975

Conversation

weaverryan commented May 19, 2023

importmap support in JsonSynchronizer

add-lines Configurator

weaverryan commented May 22, 2023

javiereguiluz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

weaverryan commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented May 26, 2023

importmap support in `JsonSynchronizer`